chore: Improve test assertions in plan stability suite #2505
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2505 +/- ##
============================================
+ Coverage 56.12% 58.40% +2.28%
- Complexity 976 1436 +460
============================================
Files 119 146 +27
Lines 11743 13510 +1767
Branches 2251 2350 +99
============================================
+ Hits 6591 7891 +1300
- Misses 4012 4389 +377
- Partials 1140 1230 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andygrove
changed the title
andygrove
changed the title
| val message = | ||
| s"Expected $planType plan in ${expectedFile.getAbsolutePath} did not match " + | ||
| s"actual $planType plan in ${actualFile.getAbsolutePath}" | ||
| throw new ComparisonFailure(message, expected, actual) |
There was a problem hiding this comment.
One problem with ComparisonFailure may be that it logs everything in a single string. Perhaps if the expected and actual had some newlines at the beginning and at the end, then the output might be slightly easier to read?
There was a problem hiding this comment.
Thanks. I reverted to the previous approach of calling fail with the formatted plans
|
|
||
| if (regenerateGoldenFiles) { | ||
| generateGoldenFile(plan, query + suffix, explain) | ||
| generateGoldenFile(dir, simplified, explain) |
There was a problem hiding this comment.
generateGoldenFile doesn't refer to name? prev implementaton referred to query + suffix
There was a problem hiding this comment.
the name (query + suffix) was previously used in generateGoldenFile to create the dir but now we just pass that in, since it has already been computed.
4 participants
Which issue does this PR close?
N/A
Rationale for this change
In the plan stability suite, there is a check that the expected and actual simplified and explain plans match, but only the simplified plan gets logged. This means that when the actual vs expected explain plan is different, no information is logged to show the difference. I discovered this while working on #2492
I split these changes out into a separate PR from #2492 to make reviews easier.
What changes are included in this PR?
How are these changes tested?